Skip to content

build: Allow custom sysroot in native_sim builds #90357

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yperess
Copy link
Collaborator

@yperess yperess commented May 22, 2025

When running naive sim builds, any custom cflags and toolchains we've set are lost. Specifically for us this included a custom directory for the sysroot and archive tool.

Add the sysroot (if available) to both NSI_BUILD_C_OPTIONS and a new NSI_EXTRA_LINK_OPTIONS. Additionally pass CMAKE_AR to NSI_AR.

@aescolar aescolar assigned aescolar and unassigned tejlmand May 23, 2025
@aescolar
Copy link
Member

@yperess would you be so kind as to clarify a bit more what you would like to do?
Are you using gcc, or clang or something else alltogether? are you trying to cross-compile? and/or just trying to use a different C library than glibc or picolibc or minimal? (or a different runtime)
At this point all I can say is that the current approach in this PR parsing for the sysroot command line option in the top level cmake file does not seem right. But I'm not able to say what would be a good way without knowing more about what you'd like to achieve.

@yperess
Copy link
Collaborator Author

yperess commented May 28, 2025

@aescolar happy to and thank you for being so responsive on this issue. This is what I'm trying to get passing https://pigweed-review.git.corp.google.com/c/pigweed/pigweed/+/293592. This chage calls twister for the Pigweed project and tests all the Zephyr backends. The important details for this call are:

The environment variables are set here

def _env_with_zephyr_vars(ctx: PresubmitContext) -> dict:
    """Returns the environment variables with ... set for Zephyr."""
    env = os.environ.copy()
    # Set some variables here.
    env['ZEPHYR_BASE'] = str(ctx.package_root / 'zephyr')
    env['ZEPHYR_MODULES'] = str(ctx.root)
    env['ZEPHYR_TOOLCHAIN_VARIANT'] = 'llvm'
    return env

Then the call:

def zephyr_build(ctx: PresubmitContext) -> None:
    """Run Zephyr compatible tests"""
    # Install the Zephyr package
    build.install_package(ctx, 'zephyr')
    # Configure the environment
    env = _env_with_zephyr_vars(ctx)
    sysroot_dir = (
        ctx.pw_root
        / 'environment'
        / 'cipd'
        / 'packages'
        / 'pigweed'
        / 'clang_sysroot'
    )
    platform_filters = (
        ['-P', 'native_sim']
        if platform.system() in ['Windows', 'Darwin']
        else []
    )
    # Run twister
    call(
        'pw',
        'twister-runner',
        '-vvv',
        '--ninja',
        '--integration',
        '--clobber-output',
        '--inline-logs',
        '--verbose',
        '--coverage',
        '--coverage-basedir',
        str(ctx.pw_root),
        *platform_filters,
        f'-x=TOOLCHAIN_C_FLAGS=--sysroot={sysroot_dir}',
        f'-x=TOOLCHAIN_LD_FLAGS=--sysroot={sysroot_dir}',
        '--testsuite-root',
        str(ctx.pw_root),
        env=env,
    )

Pigweed sets up a hermetic environment so things don't get installed in the "normal" places. Where ever you download pigweed into is your ${PW_ROOT} from there, all the dependencies are installed:

PW_ROOT
├── environment/cipd/packages/pigweed/bin/clang
├── environment/cipd/packages/pigweed/clang_sysroot/
├── environment/packages/zephyr/

So you can see everything is under the environment/ subdirectory. The twister runner is a wrapper script that sanitizes the call because Zephyr is also a subdirectory, just running twister on ${PW_ROOT} would lead to running all the upstream tests too.

Some links for reference:

@aescolar
Copy link
Member

@yperess thanks. Now I understand. I'm guessing that clang installation does not have as default sysroot that ${PW_ROOT}/environment/cipd/packages/pigweed/clang_sysroot/?
Are you passing it to the compiler thru TOOLCHAIN_C_FLAGS because Zephyr's SYSROOT_DIR did not work?

Maybe we should fix setting SYSROOT_DIR for clang (and maybe as a bonus also for the host gcc) and when it is set have it passed to the native sim runner build(?)

@yperess
Copy link
Collaborator Author

yperess commented May 28, 2025

@aescolar I think I need more context, I know my way around the Zephyr build but I'm not an expert by any means. Let's chat on discord to avoid polluting this PR comments.

@yperess yperess force-pushed the peress/native_sim_makefile branch from c17dd88 to 09014f5 Compare May 29, 2025 03:42
@yperess yperess marked this pull request as draft May 29, 2025 03:43
@github-actions github-actions bot added the area: Toolchains Toolchains label May 29, 2025
@github-actions github-actions bot requested a review from stephanosio May 29, 2025 03:43
@yperess yperess force-pushed the peress/native_sim_makefile branch 8 times, most recently from 767bed2 to 12d2e2d Compare May 29, 2025 06:37
When running naive sim builds using clang, respect user passed
SYSROOT_DIR values. Additionally, allow overriding the archive tool
when calling Make for the native simulator.

Add the sysroot (if available) to the native_simulator target and
TOOLCHAIN_C_FLAGS and TOOLCHAIN_LD_FLAGS. Additionally pass CMAKE_AR
to NSI_AR.

Signed-off-by: Yuval Peress <[email protected]>
@yperess yperess force-pushed the peress/native_sim_makefile branch from 12d2e2d to e5b91f2 Compare May 29, 2025 07:41
@yperess yperess marked this pull request as ready for review May 29, 2025 07:49
@yperess
Copy link
Collaborator Author

yperess commented May 29, 2025

@aescolar this is much better, thank you for helping clean this PR up. It passed my CI downstream using https://pigweed-review.git.corp.google.com/c/pigweed/pigweed/+/293592 (Check https://ci.chromium.org/ui/p/pigweed/builders/pigweed.try/pigweed-linux-zephyr/b8713553788173861361/overview).

Once this PR merges I will clean up my downstream change and we'll be good to go.

@yperess yperess requested review from pdgendt and aescolar May 29, 2025 07:51
Copy link

@aescolar
Copy link
Member

Looking good to me on a very quick look. Will re-review a bit later.
@tejlmand added as assignee for the cmake changes.

Copy link
Member

@aescolar aescolar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+2
Thanks @yperess it looks good to me.

@yperess
Copy link
Collaborator Author

yperess commented Jun 3, 2025

Hi @pdgendt & @tejlmand , friendly ping on the review here. Thanks.

@pdgendt
Copy link
Collaborator

pdgendt commented Jun 3, 2025

Hi @pdgendt & @tejlmand , friendly ping on the review here. Thanks.

I could only really comment on the styling, not experienced with the content of the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Build System area: native port Host native arch port (native_sim) area: Toolchains Toolchains
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants